feat: initial SyncMmr impl by drahnr · Pull Request #1636 · 0xMiden/miden-node

drahnr · 2026-02-03T15:13:31Z

Why

SynState is too broad and complicated.

What

Obsoletes SynState RPC endpoint
Adds SyncChainMmr RPC endpoint

Context

Closes #1591

Copilot

Pull request overview

This pull request deprecates the overly broad SyncState RPC endpoint and introduces a new, more focused SyncChainMmr endpoint for syncing chain MMR data, addressing issue #1591.

Changes:

Deprecated the SyncState RPC endpoint in favor of more specialized endpoints
Added new SyncChainMmr RPC endpoint to sync chain MMR deltas within a specified block range
Implemented supporting infrastructure including error types, tests, and endpoint limits configuration

Reviewed changes

Copilot reviewed 9 out of 11 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
proto/proto/rpc.proto	Marked `SyncState` as deprecated and added `SyncChainMmrRequest`/`SyncChainMmrResponse` message definitions
proto/proto/internal/store.proto	Deprecated internal `SyncState` RPC and added `SyncChainMmr` RPC endpoint
crates/store/src/state/sync_state.rs	Implemented `sync_chain_mmr` method to compute MMR deltas for given block ranges
crates/store/src/server/rpc_api.rs	Added RPC handler for `SyncChainMmr` with parameter validation and error handling
crates/store/src/errors.rs	Introduced `SyncChainMmrError` enum for request validation errors
crates/rpc/src/server/api.rs	Added passthrough implementation and endpoint limit configuration for `SyncChainMmr`
crates/rpc/src/tests.rs	Added test for `SyncChainMmr` endpoint and verified it appears in limits response
crates/rpc/Cargo.toml	Enabled `rocksdb` feature for `miden-node-store` in dev-dependencies to support integration tests
crates/proto/src/generated/store.rs	Auto-generated gRPC client/server code for the new endpoint
crates/proto/src/generated/rpc.rs	Auto-generated gRPC client/server code for the new endpoint

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

proto/proto/rpc.proto

CHANGELOG.md

proto/proto/rpc.proto

crates/store/src/server/rpc_api.rs

crates/store/src/errors.rs

Mirko-von-Leipzig

Loving the line delta :)

CHANGELOG.md

proto/proto/rpc.proto

Mirko-von-Leipzig · 2026-02-10T08:44:51Z

crates/store/src/state/sync_state.rs

+        // - Mmr::get_delta is inclusive, whereas the sync request block_from is defined to be
+        //   exclusive, so the from_forest has to be adjusted with a +1.


I worry about this definition. Users will expect (I think) MMR to return the values as per the block header number for that block, and get the mmr as at that block. Unless I'm misunderstanding.

Happy to change, kept it in-line with the previous SynState.

From a practicality perspective, when calling I have my chain-tip and want everything starting from there, so I think the common call is to do local_chain_tip.. as the request. The response I'd expect is { (local_chain_tip+1)..=(local_chain_tip+PAGE_SIZE), Mmr for <<that range }

Thing is then you can't sync from genesis though? Though I guess you have to fetch genesis header and build your own from there which you need to do to compare the hashes in any case

Fair, @igamigo is that an issue/a lot of extra work? We can also be redundent and return including the local_chain_tip (client perspective).

To expand a bit on expected usage:

Assuming there are more entries in the MMR than PAGE_SIZE, the next call would be:
local_chain_tip+1+PAGE_SIZE.. yielding (local_chain_tip+1+PAGE_SIZE)..=(local_chain_tip+PAGE_SIZE*2), Mmr for <<that range } where local_chain_tip+PAGE_SIZE is part of the pagination info returned.

I don't think we can return the response here starting on local_chain_tip, not without making it more inconvenient to apply the changes within the client anyway. Basically, we need to store peaks for a specific block (e.g, for block number 10 we want to keep the peaks of the MMR with 11 leaves). For this, when syncing, we basically apply the MmrDelta (which gets your MMR to block number 9), then apply the individual block header (number 10), and get the peaks to store alongside the block header. If on a subsequent response we got the MmrDelta starting block number 10 (whereas currently we would receive it starting block 11); we'd need to "rolllback" the MMR and apply the delta accordingly.
TLDR, I believe the current way is fine and overlapping the delta with the response/request tips may complicate things.

crates/store/src/server/rpc_api.rs

crates/rpc/src/server/api.rs

igamigo

LGTM in terms of implementation! Left a couple of questions but I don't think they are blocking

bin/stress-test/src/store/mod.rs

igamigo · 2026-02-13T14:43:36Z

crates/store/src/server/rpc_api.rs

+        &self,
+        request: Request<proto::rpc::SyncChainMmrRequest>,
+    ) -> Result<Response<proto::rpc::SyncChainMmrResponse>, Status> {
+        const MAX_BLOCKS: u32 = 1000;


Is this limit based on anything? I don't think it meaningfully caps the response size. It would probably be less overhead in terms of walking the MMR but I wonder if this is below what would be sensible since it should be logarithmic.

We can always up it to u32::MAX (as limited by PageInfo response type). The size of Mmr is a set of roots + depth of tree (so yes, log2(n)) to be transferred which should be fine for u32::MAX (or beyond if we change the representation of PageInfo).

bobbinth

I know this has already been merged, but I left some comments inline and it would be god to address them in a follow-up PR.

bobbinth · 2026-02-17T01:47:39Z

proto/proto/rpc.proto

-    // part of hashes. Thus, returned data contains excessive notes, client can make
-    // additional filtering of that data on its side.
-    rpc SyncState(SyncStateRequest) returns (SyncStateResponse) {}
+    rpc SyncChainMmr(SyncChainMmrRequest) returns (SyncChainMmrResponse) {}


Let's add some basic doc comments here (similar to how we do it for other endpoints in this file).

bobbinth · 2026-02-17T01:49:42Z

docs/external/src/rpc.md

We should probably add a subsection for SynChainMmr endpoint here.

bobbinth · 2026-02-17T01:52:07Z

docs/external/src/rpc.md

  "endpoints": {
    "CheckNullifiers": { "parameters": { "nullifier": 1000 } },
    "SyncNullifiers": { "parameters": { "nullifier": 1000 } },
-    "SyncState": { "parameters": { "account_id": 1000, "note_tag": 1000 } },
+    "SyncTransactions": { "parameters": { "account_id": 1000 } },
+    "SyncAccountVault": { "parameters": { "account_id": 1000 } },
+    "SyncAccountStorageMaps": { "parameters": { "account_id": 1000 } },
    "SyncNotes": { "parameters": { "note_tag": 1000 } },
    "GetNotesById": { "parameters": { "note_id": 100 } }


Shouldn't SyncChainMmr also be in this list?

I don't think it should, since it's paginated over the block range, so whatever requests a piece is returned with pagination info required to resume

bobbinth · 2026-02-17T01:56:36Z

crates/utils/src/limiter.rs

+/// Used for the following RPC endpoints:
+/// * `sync_chain_mmr`
+///
+/// Capped at 1000 blocks to keep MMR deltas within the 4 MB payload budget.
+pub struct QueryParamBlockRangeLimit;


Is the computation here correct? Feels like we should be able to query for way more blocks before before we hit the 4 MB limit (and since we are not sending authentication paths, I'm actually not sure we'll ever hit the limit).

Also, 1000 bocks is current about ~1 hour. If we do keep this limit, we should bump it up significantly so that a single request could cover multiple months.

This accidentally made it in. It's not used.

bobbinth · 2026-02-17T01:57:23Z

crates/store/README.md

Similar to my other comment: we should probably add a section for SyncChainMmr here.

bobbinth · 2026-02-17T02:01:02Z

crates/rpc/README.md

Similar to some other comments: this file should probably have a section on SyncChainMmr endpoint.

bobbinth · 2026-02-17T02:03:12Z

crates/rpc/src/server/api.rs

+    async fn sync_chain_mmr(
        &self,
-        request: Request<proto::rpc::SyncStateRequest>,
-    ) -> Result<Response<proto::rpc::SyncStateResponse>, Status> {
+        request: Request<proto::rpc::SyncChainMmrRequest>,
+    ) -> Result<Response<proto::rpc::SyncChainMmrResponse>, Status> {
        debug!(target: COMPONENT, request = ?request.get_ref());

-        check::<QueryParamAccountIdLimit>(request.get_ref().account_ids.len())?;
-        check::<QueryParamNoteTagLimit>(request.get_ref().note_tags.len())?;
-
-        self.store.clone().sync_state(request).await
+        self.store.clone().sync_chain_mmr(request).await


Even though we define a limit for block range, it doesn't seem like we enforce it here (unless I'm missing something). But then again, I'm not sure we actually need this limit.

The block range limit was accidentally committed and doesn't make sense for paged request

bobbinth · 2026-02-17T02:04:29Z

crates/store/src/server/rpc_api.rs

+        // TODO find a reasonable upper boundary
+        const MAX_BLOCKS: u32 = 1 << 20;


Related to some other comments - I'm not sure there needs to be an upper bound.

If there is no upper bound, even pagination, then we should remove pagination altogether.

I think the main intent for specifying the block range was so that the client could request to sync to a specific block height (rather than the chain tip). Basically, the client needs to be able to say: I want chain MMR delta starting from block $x$ and up to block $y$. But this is technically not pagination.

cc @igamigo in case I'm off on this.

Yes, that's mainly it. I think with the new flow we'd get relevant block headers through SyncNotes and then we can SyncMmr as needed.

drahnr · 2026-02-17T09:48:41Z

Created #1682 for follow-ups

drahnr requested a review from Copilot February 3, 2026 15:13

Copilot started reviewing on behalf of drahnr February 3, 2026 15:14 View session

Copilot AI reviewed Feb 3, 2026

View reviewed changes

drahnr commented Feb 6, 2026

View reviewed changes

proto/proto/rpc.proto Outdated Show resolved Hide resolved

drahnr commented Feb 6, 2026

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

drahnr marked this pull request as ready for review February 6, 2026 13:23

Mirko-von-Leipzig reviewed Feb 6, 2026

View reviewed changes

drahnr force-pushed the bernhard-obsolete-syn-state-endpoint branch from c83c375 to 3e8fdd2 Compare February 9, 2026 10:59

Mirko-von-Leipzig approved these changes Feb 10, 2026

View reviewed changes

drahnr added 14 commits February 10, 2026 16:58

initial SyncMmr impl

edc6ef0

changelog

49c5e3a

clippy + fmt

57d8190

fixup

b1eecb4

disable deprecation for now, fails clippy

88a5de8

block_num -> block.number

7088fd6

review comments

ab1e19e

fmt

8888a10

delete old RPC endpoint

d7ca3cf

limits

60efa4e

review commetns

bad68c1

more RPC docs

5164cb5

don't use block limit, ensure we use pagination properly

d9ffec8

y

76c4ecd

drahnr force-pushed the bernhard-obsolete-syn-state-endpoint branch from 7dc8b4e to 76c4ecd Compare February 10, 2026 15:58

drahnr added 3 commits February 11, 2026 15:13

Merge branch 'next' into bernhard-obsolete-syn-state-endpoint

5baead6

bench

9468d36

fix lint

05af75e

drahnr requested a review from igamigo February 13, 2026 12:36

igamigo approved these changes Feb 13, 2026

View reviewed changes

drahnr mentioned this pull request Feb 14, 2026

Add a full-sync benchmark #1676

Open

drahnr added 3 commits February 14, 2026 22:00

a bit higher

66c652c

Merge branch 'next' into bernhard-obsolete-syn-state-endpoint

9833809

upstreamfun

59163aa

drahnr merged commit dea85f7 into next Feb 16, 2026
20 checks passed

drahnr deleted the bernhard-obsolete-syn-state-endpoint branch February 16, 2026 11:58

bobbinth reviewed Feb 17, 2026

View reviewed changes

		// - Mmr::get_delta is inclusive, whereas the sync request block_from is defined to be
		// exclusive, so the from_forest has to be adjusted with a +1.

		// TODO find a reasonable upper boundary
		const MAX_BLOCKS: u32 = 1 << 20;

Conversation

drahnr commented Feb 3, 2026

Why

What

Context

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Mirko-von-Leipzig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drahnr Feb 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

igamigo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drahnr Feb 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bobbinth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drahnr Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drahnr Feb 10, 2026 •

edited

Loading

drahnr Feb 14, 2026 •

edited

Loading

drahnr Feb 17, 2026 •

edited

Loading